URL: Fix getQueryString incorrect handling of hash fragment #20738

aduth · 2020-03-09T19:13:17Z

Previously: #11286
Extracted from: #20693

This pull request seeks to fix an incorrect behavior of @wordpress/url getQueryString, where a result may be returned when a ? is encountered within the hash fragment of a URL:

getQueryString( 'https://example.com/#?foo' );
// Before: "foo"
// After:  undefined

A hash fragment is always the last segment of a URL, and the querystring must always precede it (see reference specification).

Testing Instructions:

Ensure unit tests pass:

npm run test-unit packages/url

github-actions · 2020-03-09T19:17:58Z

Size Change: +353 B (0%)

Total Size: 864 kB

Filename	Size	Change
`build/block-editor/index.js`	104 kB	-130 B (0%)
`build/block-editor/style-rtl.css`	10.7 kB	+112 B (1%)
`build/block-editor/style.css`	10.7 kB	+116 B (1%)
`build/block-library/editor-rtl.css`	7.23 kB	-124 B (1%)
`build/block-library/editor.css`	7.24 kB	-123 B (1%)
`build/block-library/index.js`	116 kB	+499 B (0%)
`build/block-library/style-rtl.css`	7.38 kB	-90 B (1%)
`build/block-library/style.css`	7.39 kB	-90 B (1%)
`build/blocks/index.js`	57.7 kB	+5 B (0%)
`build/components/index.js`	191 kB	+196 B (0%)
`build/components/style-rtl.css`	15.5 kB	-4 B (0%)
`build/components/style.css`	15.5 kB	-4 B (0%)
`build/editor/style-rtl.css`	3.97 kB	-12 B (0%)
`build/editor/style.css`	3.96 kB	-12 B (0%)
`build/url/index.js`	4.01 kB	+14 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.01 kB	0 B
`build/annotations/index.js`	3.43 kB	0 B
`build/api-fetch/index.js`	3.39 kB	0 B
`build/autop/index.js`	2.58 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.02 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-library/theme-rtl.css`	669 B	0 B
`build/block-library/theme.css`	671 B	0 B
`build/block-serialization-default-parser/index.js`	1.65 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/compose/index.js`	5.75 kB	0 B
`build/core-data/index.js`	10.6 kB	0 B
`build/data-controls/index.js`	1.03 kB	0 B
`build/data/index.js`	8.22 kB	0 B
`build/date/index.js`	5.36 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.06 kB	0 B
`build/edit-post/index.js`	91.3 kB	0 B
`build/edit-post/style-rtl.css`	8.64 kB	0 B
`build/edit-post/style.css`	8.64 kB	0 B
`build/edit-site/index.js`	4.64 kB	0 B
`build/edit-site/style-rtl.css`	2.48 kB	0 B
`build/edit-site/style.css`	2.48 kB	0 B
`build/edit-widgets/index.js`	4.44 kB	0 B
`build/edit-widgets/style-rtl.css`	2.58 kB	0 B
`build/edit-widgets/style.css`	2.58 kB	0 B
`build/editor/editor-styles-rtl.css`	381 B	0 B
`build/editor/editor-styles.css`	382 B	0 B
`build/editor/index.js`	43.8 kB	0 B
`build/element/index.js`	4.45 kB	0 B
`build/escape-html/index.js`	734 B	0 B
`build/format-library/index.js`	7.09 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	1.92 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.49 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.3 kB	0 B
`build/keycodes/index.js`	1.68 kB	0 B
`build/list-reusable-blocks/index.js`	2.99 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	4.85 kB	0 B
`build/notices/index.js`	1.57 kB	0 B
`build/nux/index.js`	3.01 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.54 kB	0 B
`build/primitives/index.js`	1.49 kB	0 B
`build/priority-queue/index.js`	780 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.3 kB	0 B
`build/server-side-render/index.js`	2.55 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/viewport/index.js`	1.61 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.18 kB	0 B

_{compressed-size-action}

aduth · 2020-03-09T19:45:08Z

As seen with the changes in 0a6046a, the new implementation is slightly more strict in regards to assuming the input argument to be a proper URL, not merely any string which happens to contain ?. While this could limit flexibility, I believe it's an appropriate change given:

getQueryString is specifically documented to receive "the full URL".
Consistency with other @wordpress/url functions which already use and rely on the URL constructor to operate against an input (e.g. URL: Conform to URL Living Standard definition of valid URL #19871).

gziolo

It looks correct. I hope it doesn’t break anything in projects that depend on this bug 😅

Previously _sort of_ existed, but as an invalid URL which would fail to construct as `new URL` in the current implementation. Now split to two assertions verifying "invalid URL" behavior and "empty querystring" behavior.

aduth · 2020-03-13T20:35:43Z

As seen with the changes in 0a6046a, the new implementation is slightly more strict in regards to assuming the input argument to be a proper URL, not merely any string which happens to contain ?. While this could limit flexibility, I believe it's an appropriate change given:

Note that I had to scale this back a bit in #20693. Specifically, there's at least a few occurrences where we rely on an implementation like this:

addQueryArgs( '/wp-json/wp/v2/posts`, { type: 'post' } );

And if @wordpress/url only supports full URLs, it loses a lot of its usefulness.

The solution (8dfe648) is a bit hacky, effectively treating the input URL as the path of a throwaway base (second argument of URL constructor).

URL: Fix getQueryString incorrect handling of hash fragment

13c8022

aduth added [Type] Bug An existing feature does not function as intended [Package] Url /packages/url labels Mar 9, 2020

aduth requested a review from talldan March 9, 2020 19:13

aduth mentioned this pull request Mar 9, 2020

URL: Implement custom getQueryArgs, buildQueryString #20693

Merged

URL: Update getQueryString test cases to proper URLs

0a6046a

gziolo approved these changes Mar 12, 2020

View reviewed changes

URL: Add test case for empty querystring as undefined

f83dc4a

Previously _sort of_ existed, but as an invalid URL which would fail to construct as `new URL` in the current implementation. Now split to two assertions verifying "invalid URL" behavior and "empty querystring" behavior.

aduth merged commit 3233009 into master Mar 12, 2020

aduth deleted the fix/url-get-query-string-hash-order branch March 12, 2020 13:22

github-actions bot added this to the Gutenberg 7.8 milestone Mar 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

URL: Fix getQueryString incorrect handling of hash fragment #20738

URL: Fix getQueryString incorrect handling of hash fragment #20738

aduth commented Mar 9, 2020

github-actions bot commented Mar 9, 2020 •

edited

Loading

aduth commented Mar 9, 2020 •

edited

Loading

gziolo left a comment

aduth commented Mar 13, 2020 •

edited

Loading

URL: Fix getQueryString incorrect handling of hash fragment #20738

URL: Fix getQueryString incorrect handling of hash fragment #20738

Conversation

aduth commented Mar 9, 2020

github-actions bot commented Mar 9, 2020 • edited Loading

aduth commented Mar 9, 2020 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Mar 13, 2020 • edited Loading

github-actions bot commented Mar 9, 2020 •

edited

Loading

aduth commented Mar 9, 2020 •

edited

Loading

aduth commented Mar 13, 2020 •

edited

Loading